Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Student Statistics Dashboard #234

Merged
merged 13 commits into from
Nov 25, 2024

Conversation

piotr-pajak
Copy link
Member

@piotr-pajak piotr-pajak commented Nov 18, 2024

Jire issue

LC-367

Preview

Screenshot 2024-11-21 at 17 27 11

@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from f680ebb to 1cbd219 Compare November 19, 2024 13:17
@piotr-pajak piotr-pajak self-assigned this Nov 20, 2024
@piotr-pajak piotr-pajak marked this pull request as draft November 20, 2024 14:18
@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch 2 times, most recently from 22ca380 to a8e6305 Compare November 21, 2024 11:32
@piotr-pajak piotr-pajak marked this pull request as ready for review November 21, 2024 14:00

const daysDiff = differenceInDays(today, lastActivityDate);

if (daysDiff === 0) return currentStats?.currentStreak ?? 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use switch case here, it will be clearer and will work better if there are more conditions in the future

switch (daysDiff) { case 0: return currentStats?.currentStreak ?? 1; case 1: return (currentStats?.currentStreak ?? 0) + 1; default: return 1; }

description: string;
itemsCount: number;
itemsCompletedCount?: number;
lessonProgress?: "completed" | "in_progress" | "not_started";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to create a type for this
type LessonProgress = "completed" | "in_progress" | "not_started";

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the filename indicates, this is a generated API based on the backend schema, so I can't modify the type here 😉

size?: number;
className?: string;
};

export const Gravatar = ({ email, size = 200, className = "" }: GravatarProps) => {
const hash = CryptoJS.MD5(email.toLowerCase().trim()).toString();
const defaultGravatarHash = "27205e5c51cb03f862138b22bcb5dc20f94a342e744ff6df1b8dc8af3c865109";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should separate defaultGravatarHash to a file where we keep constant values.

@@ -3,19 +3,21 @@ import CryptoJS from "crypto-js";
import { AvatarImage } from "./ui/avatar";

type GravatarProps = {
email: string;
email: string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

email?: string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using email: string | undefined instead of email?: string because the email field should always be included in the object, even if its value is undefined.
With email?: string, the property can be completely omitted, which doesn't meet the requirement of ensuring the field is always present.
Using email: string | undefined makes the intent clear that the property is required but can have an undefined value.

import type { ReactNode } from "react";

type ChartLegendBadgeProps = {
label: ReactNode | string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

label?: ReactNode | string
dotColor?: string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same case as above 😉

};

export const RatesChart = ({ isLoading = false, resourceName, chartData }: RatesChartProps) => {
const dataMax = Math.max(...chartData.map((d) => d.started));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the name to something more descriptive. d -> data.

Also i think it is a good idea to use useMemo in these 4 variables

<Skeleton className="h-6 w-[240px] rounded-lg" />
<Skeleton className="w-40 h-4 rounded-lg" />
</hgroup>
{/*<div className=""></div>*/}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code

@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from 82e9521 to 35d295b Compare November 21, 2024 16:40
@wielopolski wielopolski force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from 35d295b to 113d47d Compare November 22, 2024 00:08
@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch 3 times, most recently from 57a618c to af32bfd Compare November 22, 2024 09:15
@piotr-pajak piotr-pajak added the review me 👀 PR is ready to be reviewed label Nov 22, 2024
@@ -0,0 +1,13 @@
type Data =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to add object to the union just to satisfy typescript, which is not ideal solution but it will work until we figure out how to generate better types from typebox schema

<div className="flex items-center flex-col-reverse 2xl:flex-row gap-y-4 h-full gap-x-7 2xl:h-[calc(100dvh-161px)]">
<div className="w-full h-full gap-y-4 2xl:gap-x-4 2xl:gap-y-6 flex flex-col">
<div className="flex flex-wrap 2xl:flex-nowrap gap-4 w-full h-full">
<ContinueLearningCard isLoading={isLoading} lesson={userStatistics?.lastLesson} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use suspense hook for statistics or handle undefined value inside component - I noticed that there is an ts error 😉

@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch 2 times, most recently from 8805632 to 00338a4 Compare November 22, 2024 11:12
@@ -23,6 +23,11 @@ export default function Summary({ lesson }: SummaryProps) {

const lessonItemsSummary = getSummaryItems(lesson);

console.log({ lesson });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove console.logs

@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from 290f5c7 to c24bd54 Compare November 25, 2024 13:44
@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from c24bd54 to 2ef949f Compare November 25, 2024 13:55
showOutsideDays
fixedWeeks
weekStartsOn={1}
dates={Object.keys(streak?.activityHistory || {}) ?? []}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will "check" every date even if its false.
What do you think about something like this?

Suggested change
dates={Object.keys(streak?.activityHistory || {}) ?? []}
dates={keys(pickBy(streak?.activityHistory, Boolean))}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can safely apply it in the new PR to avoid possible breaking bugs before the demo 😉

@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch 2 times, most recently from da3bb7e to 10d992e Compare November 25, 2024 19:45
@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch 2 times, most recently from fa7fcf5 to ea88cca Compare November 25, 2024 19:47
@piotr-pajak piotr-pajak force-pushed the pp_mw_feat_367_student_statistics_dashboard branch from ea88cca to e22f6e6 Compare November 25, 2024 19:56
@piotr-pajak piotr-pajak added merge me 💚 PR is ready to be merged and removed review me 👀 PR is ready to be reviewed labels Nov 25, 2024
@piotr-pajak piotr-pajak merged commit 0a143bd into main Nov 25, 2024
6 checks passed
@piotr-pajak piotr-pajak deleted the pp_mw_feat_367_student_statistics_dashboard branch November 25, 2024 19:58
@piotr-pajak piotr-pajak restored the pp_mw_feat_367_student_statistics_dashboard branch November 26, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me 💚 PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants